DataGrid: Fix the banded Columns' vertical borders (T1318812)#32802
DataGrid: Fix the banded Columns' vertical borders (T1318812)#32802Alyar666 wants to merge 5 commits intoDevExpress:26_1from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes missing/incorrect vertical borders for banded (multi-row) DataGrid header cells across themes, and adds E2E coverage to prevent visual regressions.
Changes:
- Updated Material/Generic/Fluent SCSS to render vertical borders for multi-row header cells when column lines are disabled, including RTL-specific handling.
- Reorganized band-columns E2E tests by separating functional coverage and adding a new screenshot “matrix” test for RTL/LTR × showColumnLines combinations.
- Renamed the existing visual fixture and removed the unrelated functional scenario from the visual test file.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme-scss/scss/widgets/material/gridBase/layout/cell.scss | Adjust multi-row header border rules and add RTL override to fix band header vertical borders. |
| packages/devextreme-scss/scss/widgets/generic/gridBase/layout/cell.scss | Add multi-row header border rules + RTL override for consistent band header borders. |
| packages/devextreme-scss/scss/widgets/fluent/gridBase/layout/cell.scss | Replace previous multi-row header border logic with new border rules + RTL override. |
| e2e/testcafe-devextreme/tests/dataGrid/common/bandColumns/visual.ts | Keep only visual runtime-change scenario; rename fixture; remove unrelated functional test. |
| e2e/testcafe-devextreme/tests/dataGrid/common/bandColumns/matrix.ts | Add screenshot matrix coverage for vertical borders across showColumnLines/rtlEnabled permutations. |
| e2e/testcafe-devextreme/tests/dataGrid/common/bandColumns/functional.ts | Move the columnOption/dataField functional regression test into a dedicated functional file. |
| configs.forEach(( | ||
| { showColumnLines, rtlEnabled }: { showColumnLines: boolean; rtlEnabled: boolean; }, | ||
| ): void => { | ||
| test('test', async (t) => { | ||
| const { takeScreenshot, compareResults } = createScreenshotsComparer(t); | ||
| const dataGrid = new DataGrid(GRID_CONTAINER); | ||
|
|
||
| await t.expect(dataGrid.isReady()).ok(); |
There was a problem hiding this comment.
The generated tests all use the same title ('test'). This makes TestCafe output hard to interpret and can also produce ambiguous reporting when multiple tests fail. Please use a descriptive, unique test name (e.g., include the T1318812 reference and the current showColumnLines/rtlEnabled values).
Alyar666
force-pushed
the
T1318812_26_1_attempt_3
branch
from
88936f8 to
ccd2691
Compare
| private updateFirstHeaderClasses(): void { | ||
| const $rows = this._getRowElementsCore().toArray(); | ||
|
|
||
| $rows.forEach((row: Element, index: number) => { | ||
| const rowIndex = index; | ||
| const $cells = $(row).children('td').toArray(); | ||
| const columns = this.getColumns(rowIndex); |
There was a problem hiding this comment.
updateFirstHeaderClasses derives rowIndex from the DOM loop index. _getRowElementsCore() can include non-header rows (e.g. the filter row is added by filter/m_filter_row.ts without a rowIndex), so using the loop index can pass an invalid header rowIndex into getColumns(...)/columnsController.isFirstColumn(...) and toggle the class on the wrong cells. Consider normalizing rowIndex for non-header rows (e.g. index < this.getRowCount() ? index : null) and also ensuring _createCell doesn’t pass an undefined options.rowIndex for non-header rows.
| this.setStickyOffsets(); | ||
|
|
||
| if (columnHidingEnabled) { | ||
| this.updateColumnNoBorderClasses(); |
There was a problem hiding this comment.
updateColumnNoBorderClasses() walks all rendered rows/cells and calls needToRemoveColumnBorder per cell. After this change it runs on every _resizeCore() whenever columnHidingEnabled is true, even when no columns are actually being hidden, which can add significant resize overhead on large grids. Consider restoring a guard similar to the previous adaptiveColumns.getHidingColumnsQueue().length/hasHiddenColumns() check before doing the full traversal.
| if (hasStickyColumns && !needToDisableStickyColumn(this._columnsController, column)) { | ||
| this.updateBorderCellClasses($cell, column, rowIndex); | ||
| this.toggleColumnNoBorderClass($cell, column, rowIndex); | ||
|
|
There was a problem hiding this comment.
The sticky-columns view no longer toggles the *-first-header class on cells (the old toggleFirstHeaderClass call was removed). However, SCSS in this PR still relies on .dx-datagrid-first-header / .dx-treelist-first-header to remove the leading border in the content table. With the current code, those classes appear to be applied only in ColumnHeadersView (headers), not in RowsView/content cells, so the content-border rules may never take effect. Either reintroduce applying the first-header class for content cells (e.g. in RowsView/sticky-columns extender) or remove/update the dependent SCSS rules.
No description provided.